Docker filter hardening: 21 review findings, three endpoint families, and a log-injection fix by bfulton · Pull Request #38 · bfulton/localmost

bfulton · 2026-09-06T20:03:10Z

Hardening for the docker isolation filter merged in #35, plus the three endpoint families a real consumer needs, plus a job-start bug that turned out to be a log-injection hole.

Everything here is TDD: each fix has a test that fails first, for the right reason, and the end-to-end suite runs against a real Docker daemon after every change.

Where the work came from

A six-dimension adversarial review of the merged filter (escape/bypass, fail-closed, policy grammar, socket lifecycle, real-CLI correctness, test quality), each finding then attacked by three skeptics on distinct lenses; a majority refute killed it. 21 findings survived and all 21 are fixed here. Two more I found while reproducing them, and one arrived as consumer feedback from wiring a container-heavy repo to the shipped socket.

Sandbox escapes and filter bypasses

Anonymous volume with local-driver bind options (critical) — a Mounts entry of Type: volume with no Source skipped every mount check, but the built-in local driver with type=none,o=bind,device=<path> is a bind mount. Arbitrary host path, read-write, whatever the policy declared.
Case-folded keys (critical) — the evaluator read HostConfig.Privileged with case-sensitive property access while the daemon decodes with Go's encoding/json, which falls back to a case-insensitive field match. {"hostconfig":{"privileged":true,"binds":["/:/host:rw"]}} was invisible to every gate and honoured in full.
Upgrade tunnelling (critical) — GET /_ping is baseline, so an Upgrade header on it opened a raw pipe to the daemon; a denied GET /containers/json smuggled over that socket returned 200. Only attach upgrades now, and only after the daemon actually answers 101.
HostConfig was a blocklist (high) — so every key nobody enumerated was forwarded unexamined, PortBindings among them, publishing a container port on the operator's interfaces outside the proxy that controls the job's egress. It is an allowlist now, built from the 62 keys a real docker run sends — captured from the CLI, after two attempts guessed wrong and broke docker run outright.
Build parameters unfiltered (high) — docker build --network host walked through a door create keeps shut.
Bind-mount TOCTOU (high) — the filter resolved a symlink to decide and forwarded the job's spelling for the daemon to resolve again. The verdict now carries the body it approved, with sources pinned to the paths actually checked.

Consent and correctness

The approval screen showed no docker grants at all (high), and a bare run: {} — which permits creating and running containers — produced zero approval diff (high). An operator could approve container access without being shown any.
Registry auth was never wired (high) — the proxy could always attach X-Registry-Auth and nothing passed it the callback, so private-registry pulls could not work and "credentials stop entering the sandbox" described a capability that did not exist. Resolved in the app now, following the CLI's own order: per-registry helper, credential store, then inline auths. Docker Desktop keeps secrets in a helper, so inline-only would have covered nobody.
A drifted worker was reopened at job start (high) — the claim path cuts the network and leaves the docker socket shut; the job-start refresh runs without isClaim, never re-checks drift, and fell straight through to re-widening both. Found independently by two dimensions.
Phantom jobs from job output (high, found while reproducing) — the job-start detector matched Running job: anywhere in worker output. The merge commit for CLI target management, Docker isolation, config hardening, and App permission docs #35 contains that literal text, so checking it out made localmost read three job starts that never happened. Any repo could do it deliberately with a commit message.
Malformed request targets threw out of the handler and hung the connection; a released slot kept the finished job's context, so the next worker was judged against the previous repository; --name containers were refused though the job created them, while a removed container's prefix still opened the door; mounts rooted at _work instead of the checkout, making any path narrower than ./ unmatchable; per-workflow keys resolved against github.workflow (the workflow's name:) rather than the filename the docs promise.

The three endpoint families (consumer feedback)

Networks — the one that mattered most, because the direction was backwards: an --internal network makes a container less reachable, so denying network creation forced strictly weaker isolation than the workload wanted. Two corrections to the original proposal: the driver is unnameable (macvlan/ipvlan puts a container on the physical LAN, worse than --network=host), and NetworkMode must accept an owned network or a job could create vk-1 and then be refused when it tried to use it.
Image inspect — scoped by the policy rather than a second ownership ledger: inspecting an image run.images already names discloses nothing new, and the container ledger had already produced one defect of exactly that kind.
kill / stop / logs — stop and logs added beyond the ask; refusing logs contradicted the documented "reads about the job's own containers" baseline.

What wiring a real consumer up to it found

Four more rounds came back from a container-heavy eval harness pointed at the filtering socket. Every one was the same failure mode — a rule that reads wider than it matches — and each was measured on the wire, not read off the spec.

The allowlist was built from the spec, not the wire. docker network create was refused outright, because the CLI always sends keys the allowlist had never seen. Same class as the earlier ContainerIDFile and PortBindings breaks: the fix is a captured body, not a careful reading. Keys the CLI always sends are now value-gated rather than absent — present-and-inert passes, present-and-set is refused by name.
* crossed /. vk/grader:* matched vk/grader:a/b. Anchored and segment-scoped now; a tag glob is unaffected, since a tag cannot contain a slash.
BuildKit is unfilterable, so jobs are pinned off it. A default docker build issues zero POST /build — it negotiates a gRPC session and streams the context over /grpc, so no request carries the paths it reads. Jobs now run with DOCKER_BUILDKIT=0, and /grpc gets a denial that says which builder and which variable rather than shrugging "unknown endpoint". This has a shelf life: the classic builder is deprecated.
A tagless glob covered almost nothing. vk/* allowed vk/setup-tools and denied vk/setup-tools:1 — not the glob's doing, but normalisation appending :latest to both sides. Rather than guess that :* was meant, validation refuses a tagless glob and names vk/*:* in the message, for the reason docker: true is refused rather than interpreted.

Dependabot #167

extract-zip symlink path traversal (high, dev-only). There is no patched release and never will be; npm's suggested fix is a downgrade to a breaking older forge. Aliased to @electron-internal/extract-zip, the maintained fork @electron/packager 20 uses. Overriding packager to ^20 was tried and rejected — forge 7.11.2 is the newest forge, still asks for ^18.3.5, and packaging fails with TypeError: done is not a function.

Things a reviewer should look at rather than take on trust

126bc25 is red in history. I chained the gate and the commit into one step and it committed despite a failing e2e. Fixed forward in d1757a9; it will bite a bisect.
Two deliberate behaviour changes: the daemon now receives the resolved mount path rather than the job's spelling, and mounts root at the checkout (_work/<repo>/<repo>) rather than _work — narrower than before.
privileged: true is now a validation error, not just a request-time denial. An existing .localmostrc declaring it stops parsing.
build.context was fixed in the docs, not the code. It is not enforceable — the Engine API carries the context as a tar the client already assembled, so no path reaches the filter; the seatbelt profile is what confines it. Writing a fake check would have been worse than saying so.

1076 tests, tsc and eslint clean, e2e 4/4 against a live daemon.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

The job-start detector matched "Running job: <name>" anywhere in a worker's output, and a worker's output is untrusted: it includes whatever the job prints and whatever the runner echoes of the job's own contextData. The merge commit for #35 contains that literal text in its message, so checking it out made localmost read three job starts that never happened - each one taking a history entry that stays "running" forever, firing a Job Started notification, marking the worker busy, disarming its acquire deadline and rebinding its docker socket. Any repository could do this deliberately with a commit message, branch name or PR title. Anchor the match to the start of a line, behind the runner's own optional timestamp, and refuse any start on a worker that already has a job: the runner is --once, so one spawn is exactly one job and a second start is always output, never a job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Both let a job reach the daemon unfiltered, and both are exploited in tests that fail before the fix. An Upgrade header on any permitted request opened a raw pipe. handleUpgrade forwarded whatever passed the policy check, and GET /_ping is in the always-on baseline, so a job could ask to upgrade a ping and then pipeline arbitrary Docker API calls down the resulting socket - the test smuggles a denied GET /containers/json over it and gets 200 from the daemon. Only attach is an upgrade now, and the pipe is established only once the daemon has actually answered 101; anything else is relayed and closed. Separately, the evaluator read HostConfig and its fields with case-sensitive property access while the daemon decodes them with Go's encoding/json, which falls back to a case-insensitive field match. A body saying "hostconfig": {"privileged":true,"binds":["/:/host:rw"]} was invisible to every gate and honoured in full by the daemon. Keys are now read the way the daemon reads them: every casing of a gated key must pass, every casing of Image must name a declared image, and the most restrictive NetworkMode wins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

An addendum to the stage 1 design, prompted by wiring a container-heavy repository to the shipped filter. Networks, image existence reads, and stopping an owned container all classify as `other` today and are denied. Networks matter most because the direction is backwards: an --internal network makes a container less reachable, so denying it forces strictly weaker isolation than the workload wants. The grammar declares a name glob and whether the network is internal; the driver stays unnameable, since macvlan on the physical LAN is worse than --network=host, and any create key the filter does not recognise is refused. NetworkMode must then accept an owned network, or the feature cannot be used at all. Image reads are scoped by the policy rather than by a second ownership ledger: inspecting an image run.images already names discloses nothing new, and the container ledger has already produced one defect. kill, stop and logs join the run action with the existing own-container scoping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Three endpoints classified as `other` and hit default-deny, so a job could start a container but never stop one that overran its budget, and never read what it printed. Consumer feedback asked for kill; stop belongs with it, since a timeout path that can only kill is worse than one that can ask politely first, and logs is exactly the "reads about the job's own containers" the design already documents as baseline - refusing it contradicted the docs rather than implementing them. All three reuse the scoping that is already there: kill and stop join the run action, logs joins inspect as a read, and every one of them is permitted only against a container this socket created. Design: docs/superpowers/specs/2026-09-06-docker-endpoint-families.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Two findings from the review, both letting a create body reach past the filter. An "anonymous" volume - a Mounts entry of Type volume with no Source - was treated as container-lifecycle storage and skipped every mount check. But the built-in local driver with type=none,o=bind,device=<path>, the mechanism compose exposes as driver_opts, makes that entry a bind mount of an arbitrary host path, read-write, whatever the policy declares. A volume carrying DriverConfig is now refused, and the remaining mount keys are read the way the daemon reads them. HostConfig was gated by a blocklist, so every key nobody enumerated was forwarded unexamined - PortBindings among them, publishing a container port on the operator's interfaces and outside the proxy that controls the job's egress. It is an allowlist now: a key the filter does not understand is refused, which is what the grammar already promises about itself. The allowlist is built from the 62 HostConfig keys a real docker run actually sends, captured from the CLI rather than guessed - the first two attempts broke `docker run` outright, on ContainerIDFile and then PortBindings, and the end-to-end test against a real daemon is what caught it. Keys that are only dangerous when non-empty are gated by value rather than refused outright: ContainerIDFile, PortBindings, PublishAllPorts, Cgroup, ExtraHosts, GroupAdd, Links and VolumeDriver. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

When a claim finds the approved policy has moved since the worker was built, it cuts the network back to runner infrastructure, leaves the docker socket closed and retires the worker: the job runs under the boundary that was actually approved for it. The job-start refresh then undid all of that. It calls applyPolicyForTarget without isClaim, so the drift check never runs, and it fell straight through to the widening below - restoring the full host list and binding the docker socket the claim had deliberately left shut. Found independently by two review dimensions, which is what made it worth looking at closely. The constraint is now recorded on the worker and every later refresh leaves it alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

docker build carries its whole configuration in the query string, and the filter read exactly one parameter of it. So `docker build --network host` walked through a door `create` keeps shut, and extrahosts, cachefrom, securityopt and outputs were forwarded unexamined. Build parameters are an allowlist now, on the same reasoning as HostConfig, and networkmode is held to the rule the run path already applies. build.context was reported three times as validated, merged, diffed and serialized but never enforced. It is not enforceable: the Engine API carries a build context as a tar the client already assembled, so no path reaches the filter to check. What confines a local context is the seatbelt profile, since the job can only read what the profile grants, and the filter's part is to refuse a remote context that would have the daemon fetch it instead. The spec and the docs said otherwise; they now say this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Two review findings, both meaning an operator could approve container access without being shown any. The approval screen summarises what a policy grants - network hosts, filesystem reads and writes, sockets - and had no notion of docker at all, so a policy granting a repository the daemon listed nothing for it. It now describes pulls, images, mounts, network and privileged, per workflow as well as shared. Separately the approval diff compared only the conditions inside each action block, so a bare `run: {}` - which permits creating and running containers - produced no diff entry and reached approval invisibly. An action appearing or disappearing is now itself a diff entry, reported only when the block would otherwise leave no trace, so a block that merely changed its conditions is not reported twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

The filter resolved each bind source through symlinks to decide, then forwarded the request as written, and the daemon resolved the same string again at mount time. The job can write in its own workspace, so it could point a symlink inside the workspace while the filter looked and somewhere else before the daemon did - a create that passed the check mounting /etc. The verdict now carries the body it approved, with every mount source replaced by the path that was actually resolved and checked, and that is what is forwarded. A fully resolved path resolves to itself, so the daemon's second resolution can no longer disagree with the filter's first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

The proxy has always been able to attach an X-Registry-Auth header to a pull, and nothing ever passed it the callback that produces one - so private registry pulls could not work, and the design's promise that credentials stop entering the sandbox described a capability that did not exist. Resolve them in the app, where ~/.docker is readable and stays denied to the job, following the docker CLI's own order: a per-registry credential helper, then the configured credential store, then an inline auths entry. Docker Desktop keeps its secrets in a helper rather than in the file, so supporting only the inline form would have covered almost nobody. An identity token is carried as a token rather than as a password, and a registry with no stored credential resolves to nothing rather than failing - an anonymous pull may well succeed. Naming a registry in pull.registries remains the whole grant; the job still never reads a secret. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

parseDockerRequest called new URL() on the raw target with nothing catching it, so `GET //` or `GET http://[` threw out of the http request handler: no refusal was written and the connection sat open until the client gave up. A filter that hangs is a filter that fails open in the only way that matters - the request is neither allowed nor denied, and nobody is told. Worse than the throw is the target that parses. `//evil/v1.45/containers/json` resolves here to host=evil, path=/containers/json, while the daemon reads the target as written - so the filter would judge one request and forward another. Only origin-form targets are accepted now; anything else is reported by the parser and refused with 400 naming the target, which is a bad request rather than a policy denial and reads that way to whoever hit it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

The docs say a `workflows:` key matches the workflow filename. An earlier fix in this branch moved the lookup off the scraped job name and onto github.workflow, which was closer but still wrong: github.workflow is the workflow's `name:`, a free-form string that equals the filename only by coincidence. A repository following the documented contract still saw its per-workflow section ignored. github.workflow_ref carries the real path, so the filename is taken from there and the name stays as the fallback for a job that arrives without it. Doing this in the broker means nothing else had to change: the consumer already reads githubWorkflow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

releaseInstanceSlot dropped the instance and left the finished job's target context behind under the same slot number. The next worker to take that slot was then judged against the previous repository: its docker socket refused the job it was actually running, and a worker that records no context of its own - one that picked a job up without going through spawnWorkerForJob - would resolve the previous repository's approved filesystem policy at spawn. The context describes one job in one slot, so it goes when the slot does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Two findings about container identity, from opposite directions. A job that runs `docker run --name mine` never sees an id: create carries the name as a query parameter and every later call addresses "mine". Only the id from the create response was recorded, so the job was refused access to the container it had just created. The name is recorded too now. The other way, ownership was too generous. A bare prefix of an owned id counted, on the reasoning that the daemon accepts one, and nothing was ever removed from the set - so after a job removed its container, that container's prefix, and its name, still opened the door to whatever the daemon resolved them to next on a shared machine. Identifiers must match exactly now, and a removal the daemon confirms forgets every identifier for that container. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Declared mount paths resolved against the runner's _work folder, but a job checks out into _work/<repo>/<repo> - GITHUB_WORKSPACE, and what `./` means to whoever wrote the policy. So `path: ./tmp/fixtures` resolved to _work/tmp/fixtures, which never exists: every declared path narrower than `./` was silently unmatchable. The root follows the repository the socket is bound to, and falls back to the work folder while nothing is bound. `privileged: true` also validated clean and was then refused on every request. The design keeps it in the grammar so the capability gap stays visible and rejects it until a backend can contain it, which means rejecting the declaration rather than accepting one that can never be honoured - the second reads as a broken policy instead of a stage that has not shipped. That is the last of the twenty-one findings the review confirmed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Rooting mounts at the repository checkout changed where the proxy expects a job's files to be, and the end-to-end test still built its workspace at _work. The two disagreed and two of the four cases failed - correctly: the test was describing a layout the runner does not use. It now asks the backend for the checkout path for the repository it binds, the way the proxy does, so the test and the thing under test agree about where a job's files live. Committed after the fact: the previous commit was pushed with this failing, because the gate and the commit ran in one step and the commit did not wait for the end-to-end result. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Consumer feedback, and the item worth prioritising because the direction was backwards: an --internal network makes a container less reachable, not more, so denying network creation forced strictly weaker isolation than the workload wanted. A harness that seals an agent under test behind a no-egress network could not build the thing doing the sealing. run: networks: - name: vk-* internal: true The name is an anchored glob and `internal` must be stated, so a routable network is a grant somebody asked for and the approval diff shows. The driver is unnameable: macvlan and ipvlan put a container on the physical LAN, which is worse than --network=host, and Options can bind a bridge to a host address. Any create key the filter does not recognise is refused, as with HostConfig. Reading and deleting a network is scoped to ones this socket created, by id and by the name the job asked for, and both are forgotten when the daemon confirms the delete. Listing stays denied - it enumerates the daemon. NetworkMode accepts a network this job created as well as the declared run.network. Without that the feature would be unusable: a job could create vk-1 and then be refused when it tried to run anything on it. Design: docs/superpowers/specs/2026-09-06-docker-endpoint-families.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

`docker image inspect` is the natural "do I already have this?" check, and a build-once-mount flow uses it to decide between building and pulling. It classified as `other` and was denied. Scoped by the policy rather than by a second ownership ledger, which is where the consumer's suggestion and this differ: an inspect of an image run.images already grants discloses nothing the policy has not granted, and it avoids bookkeeping that would have to reconcile pulls by tag with builds by id. The container ledger has already produced one defect of exactly that kind. Listing and deleting images stay denied: both are daemon-wide, and the consumer agrees. That completes the three endpoint families the feedback asked for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Dependabot #167 (GHSA-jmr9-qjv8-65gv, high): extract-zip does not validate symlink paths when extracting. It reaches us only through @electron-forge/cli -> @electron/packager, so it is build tooling and never ships - `npm audit --omit=dev` reports nothing - but a zip that extracts outside its destination is worth removing from a build that signs an app. There is no patched extract-zip and there never will be: 2.0.1 is the last release and the advisory lists no fix. npm's own suggestion is to downgrade to @electron-forge/cli@6.4.2, which is older and breaking - worse than the problem. Upstream's answer is @electron-internal/extract-zip, a maintained fork that calls itself a drop-in replacement, which @electron/packager 20 uses in place of the original. Aliasing the dependency to that fork applies the same fix while staying inside the range forge declares. Overriding @electron/packager to ^20 instead was tried and rejected: forge 7.11.2 is the newest forge and still asks for ^18.3.5, and packaging fails with "TypeError: done is not a function" from forge's own api/package.js. The constraint is forge, not packager's stability. Verified by packaging the app, which is the code path that unzips. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Copilot

🟡 Changes recommended

The consent/approval surfaces still omit or under-describe newly added docker grants (notably run.networks) which can make operator approval incomplete or misleading.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Hardens the Docker filtering socket introduced in #35 by expanding supported Engine API coverage (networks, image inspect, lifecycle endpoints), tightening request parsing and evaluation to close bypasses, and fixing policy/consent correctness issues in the runner runtime.

Changes:

Extends docker policy grammar and evaluator to support controlled network creation, image inspect, and additional container lifecycle endpoints (kill/stop/logs), with strict allowlisting and ownership scoping.
Hardens the proxy/evaluator against bypasses (malformed request targets, upgrade tunneling, HostConfig key allowlist, case-insensitive JSON decoding, mount TOCTOU via pinned resolved sources).
Fixes runner/runtime correctness issues (job-start output injection, policy drift reopening, stale slot context) and wires registry auth resolution in the app so credentials never enter the sandbox.

File summaries

File	Description
test/e2e/docker.spec.ts	Updates e2e setup for repo-aware workspace mount root and proxy bind.
src/shared/localmostrc.test.ts	Adjusts config serialization tests for privileged validation change.
src/shared/docker-policy.ts	Adds `run.networks`, validates/merges it, and improves approval diffing/serialization.
src/shared/docker-policy.test.ts	Adds tests for bare action-block diffing, privileged validation, and `run.networks`.
src/main/runner-manager.ts	Fixes slot context cleanup, job-start detection hardening, and policy drift stickiness.
src/main/runner-manager.test.ts	Adds regression tests for injected job-start output, drift stickiness, and slot context carryover.
src/main/ipc-handlers/policy.ts	Adds docker grants into policy approval summarization and exports `summarizeGrants`.
src/main/ipc-handlers/policy.test.ts	New tests ensuring docker grants and bare action blocks appear in approval summary.
src/main/index.ts	Wires registry auth resolution into RunnerManager via `attachRegistryAuth`.
src/main/docker/registry-auth.ts	Implements Docker credential resolution (helper → store → inline auths) and header encoding.
src/main/docker/registry-auth.test.ts	Tests credential resolution order, default registry keying, and failure behavior.
src/main/docker/docker-request.ts	Adds endpoint classification for new actions and refuses non-origin-form/malformed targets.
src/main/docker/docker-request.test.ts	Tests new endpoint mappings and target parsing refusal behavior.
src/main/docker/docker-filter-proxy.ts	Tracks network/container ownership aliases, pins approved mount paths, hardens upgrade handling, and refuses unreadable targets cleanly.
src/main/docker/docker-filter-proxy.test.ts	Adds coverage for upgrade tunneling prevention, mount pinning, unreadable targets, name-based ownership, and networks.
src/main/docker/docker-evaluator.ts	Hardens request evaluation (case-insensitive decoding behavior, HostConfig/build allowlists, build network gating, networks and image inspect).
src/main/docker/docker-evaluator.test.ts	Adds extensive regression tests for case-folding, volumes-as-binds, HostConfig allowlist, build params, networks, and image inspect.
src/main/docker/docker-backend.ts	Makes workspace mount root repo-aware (checkout-rooted) to match workflow path semantics.
src/main/docker/docker-backend.test.ts	Tests repo-aware mount root vs fallback behavior.
src/main/broker-proxy-service.ts	Derives workflow keying from `github.workflow_ref` filename (fallback to `github.workflow`).
src/main/broker-proxy-service.test.ts	Tests workflow filename extraction across extensions and ref formats.
package.json	Aliases `extract-zip` to `@electron-internal/extract-zip` fork to address advisory constraints.
package-lock.json	Locks the aliased extract-zip fork and removes now-unused transitive deps.
docs/superpowers/specs/2026-09-06-docker-endpoint-families.md	Adds design/spec addendum for the three endpoint families and rationale.
docs/superpowers/specs/2026-09-05-docker-isolation-design.md	Corrects build context enforcement claims (documentation-only + remote refusal).
docs/roadmap/localmostrc.md	Updates docs for workflow matching semantics and expanded docker action coverage/constraints.

Review details

Files reviewed: 25/26 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

# Conflicts: # src/main/broker-proxy-service.test.ts

Copilot

🟡 Changes recommended

The approval UI still omits run.networks grants and network-create evaluation is not fail-closed under Go’s case-insensitive JSON decoding, which can undermine consent and enforcement.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/main/ipc-handlers/policy.ts:56

describeDocker() doesn’t include the new run.networks grants (ability to create networks), so the approval UI can still under-report what an operator is consenting to when a policy adds networks.

  if (docker.run) {
    const { images = [], mounts = [], network } = docker.run;
    if (images.length === 0 && mounts.length === 0 && network === undefined) {
      grants.push(`${prefix}docker run`);
    }
    for (const image of images) grants.push(`${prefix}docker run image: ${image}`);
    for (const mount of mounts) grants.push(`${prefix}docker mount: ${mount.path} (${mount.mode})`);
    if (network !== undefined) grants.push(`${prefix}docker network: ${network}`);
  }

Files reviewed: 25/26 changed files
Comments generated: 2
Review effort level: Lite

Two blockers from wiring a container-heavy consumer to the filter, and four review findings. `docker network create` was refused for every stock CLI invocation. The allowlist was built from the spec rather than from real traffic, and the CLI sends Scope, IPAM, Attachable, Ingress, ConfigOnly, ConfigFrom and Options unconditionally with inert defaults - so the feature was reachable only from a hand-written API client, and the spec's own end-to-end scenario would have failed the moment it went through the CLI. They are gated by value now, the way HostConfig already treats the keys a plain `docker run` always sends: the default passes, anything meaningful is refused. This is the third time an allowlist has been written from the documentation instead of from the wire; the captured body is now in the test. `run.images` matched exactly, which excludes a content-addressed tag - the image cannot be named when the policy is written, and re-approving on every rebuild is not a workflow. Entries are anchored globs now, reusing the matcher that already backs network names, so `vk/grader:*` covers create and inspect together while `evil/vk/grader:x` still does not match. Review findings: network create now reads every casing of Name, Driver and Internal, since the daemon decodes them case-insensitively and a second casing may be the one it honours; declared networks appear in the approval-grants summary; the approval diff says internal or routable explicitly rather than leaving routable as a bare name; and DockerRunPolicy's doc comment, which I had detached by inserting DockerNetworkPolicy above it, is reattached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

A consumer measured the matcher again and found a boundary nobody had written down: `vk/*` allowed `vk/setup-tools` but denied `vk/setup-tools:1`. The glob itself is innocent - `[^/]*` spans a colon happily. It is normalisation: a reference with no tag gets `:latest` on both sides, so `vk/*` is matched as `vk/*:latest` and covers the latest tag of each repository and nothing else. Someone writing it to mean "any image in our namespace" gets a policy covering almost none of them, and an approval diff shows the wide-looking form, not the narrow meaning. Same silently-doesn't-apply class as the `build:` key that used to be ignored. Two ways out: read a tagless glob as `:*`, or refuse it. Refusing wins for the reason `docker: true` is refused rather than interpreted - the grammar does not guess at intent it can ask for. Validation now rejects a tagless glob with the form that means what it looks like in the message. Exact references are untouched: `alpine` still means `alpine:latest`, which is what it looks like. The evaluator keeps a test pinning what a tagless glob would match, so the reason the rejection exists stays visible, and the three places that claimed "`vk/*` reaches one level under `vk`" now say `vk/*:*` and name the tag boundary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Review flagged `pick()` as a case-folding bypass: a body can carry both `HostConfig` and `hostconfig`, and the filter judges one while the daemon honours another. Real, and worse than the report - the suggested fix was to read the last duplicate, on the theory that Go's decoder is last-wins. Measured against a real daemon rather than reasoned about. A create body with `HostConfig`, `hostconfig` and `HOSTCONFIG` came back carrying fields from all three: AutoRemove from the first, Memory from the second, OomScoreAdj from the third. Go decodes each key into the same struct field in document order, so nested objects MERGE; scalars and arrays within one object are last-wins. Reading the last duplicate is therefore exactly as wrong as reading the first, and getting it right means reimplementing encoding/json inside the filter. So the ambiguity is refused instead of modelled. Go's encoder emits unique, exactly-cased keys, so no real client sends a case-variant duplicate - the docker CLI's own bodies are clean, which the e2e exercises end to end. A recursive check at the evaluator's entry denies any body containing two casings of one key, naming both spellings. One check, every action with a body, fail-closed. Second finding, also real: relayNetworkCreate recorded ownership from `body.Name` case-sensitively, so a client sending `name` created a network the evaluator had approved and the proxy then refused to let it join, inspect or delete. It now reads the key the way the daemon does, which the check above makes unambiguous by construction. Container aliases are unaffected - they come from the query string, and Go's url.Values is case-sensitive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Copilot

🔵 Needs a closer look

src/main/docker/docker-evaluator.ts still has mount-parsing paths that are not fail-closed under Go’s case-insensitive JSON decoding, leaving a potential host mount bypass.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:96

resolveRegistryAuth always calls credential helpers with serveraddress=keys[0] (usually the bare registry), even if the helper was discovered under a different config key (e.g. credHelpers["https://ghcr.io"]). That can make stored credentials undiscoverable. Track which key selected the helper and use that as serveraddress when invoking the helper and encoding the auth.

This issue also appears on line 108 of the same file.

src/main/docker/docker-evaluator.ts:284

parseMount reads Mount.Type via pick(), which is not fail-closed under Go's case-insensitive JSON decoding: a request can supply multiple casings (e.g. Type vs type) with different values and the daemon may honor a different Type than the filter judged, potentially bypassing mount checks (e.g. tmpfs vs bind). Refuse when Type values disagree across casings before continuing.

  const type = pick(mount, 'Type');
  if (type === 'tmpfs') return null;

src/main/docker/docker-evaluator.ts:295

The volume-driver bypass fix is still case-sensitive: it checks VolumeOptions.DriverConfig via pick(), so a request can include both DriverConfig (null) and driverconfig (object) and potentially have the daemon honor the latter while the filter judges the former. Check every casing with valuesFor(...) and deny if any is present.

    const volumeOptions = pick(mount, 'VolumeOptions');
    if (isPlainObject(volumeOptions) && !isUnset(pick(volumeOptions, 'DriverConfig'))) {
      return 'a volume with DriverConfig is not permitted: a volume driver can bind-mount a host path, which only a declared workspace mount may do';
    }

src/main/docker/docker-evaluator.ts:297

For volume mounts, Source is checked via pick(), so a request can supply Source:"" (anonymous) alongside source:"named" and potentially have the daemon honor the named volume while the filter allows it. Refuse when any casing supplies a non-empty Source value (and validate types), rather than only the picked one.

    if (isEmptyString(pick(mount, 'Source'))) return null; // anonymous: lives with the container
    return `"${String(pick(mount, 'Source'))}" is a named volume, not a workspace path; only declared workspace mounts are permitted`;

src/main/docker/docker-evaluator.ts:303

For bind mounts, Source is selected via pick(), so multiple casings with different values (e.g. Source:"/safe" and source:"/") can cause the filter to validate one path while the daemon mounts another. Refuse when non-unset Source values disagree across casings before proceeding.

  const source = pick(mount, 'Source');
  if (typeof source !== 'string' || !path.isAbsolute(source)) {
    return 'a bind mount needs an absolute Source';
  }

src/main/docker/docker-evaluator.ts:310

BindOptions.Propagation is read via pick(), so conflicting casings can bypass the propagation allowlist (the daemon may honor a different casing/value than the filter checked). Validate every casing with valuesFor(...) so any disallowed value is refused.

    if (!isPlainObject(options)) return 'BindOptions must be an object';
    const propagation = pick(options, 'Propagation') ?? '';
    if (!PROPAGATIONS.has(propagation as string)) {
      return `mount propagation "${String(propagation)}" is not permitted`;
    }

src/main/docker/registry-auth.ts:112

When falling back to inline auths entries, the code encodes serveraddress=keys[0] even if the matching auth entry came from a different key (e.g. https:///v1/). That can break auth on registries whose config key isn’t the bare hostname. Encode the serveraddress using the specific auth-entry key that matched.

  for (const key of keys) {
    const entry = config.auths?.[key];
    if (!entry) continue;
    if (entry.identitytoken) return encode({ identitytoken: entry.identitytoken, serveraddress });
    if (!entry.auth) continue;

Files reviewed: 26/27 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🟡 Changes recommended

It introduces at least one committed generated artifact (test-results/.last-run.json) and a Jest test file that uses jest.* without importing jest from @jest/globals, which can break test execution in ESM mode.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/ipc-handlers/policy.test.ts:1

This test uses jest.mock/jest.fn but doesn’t import jest from @jest/globals. In Jest’s ESM mode, jest is not injected as a global, so this can fail at runtime; other tests in this repo import jest explicitly when needed.

Files reviewed: 26/27 changed files
Comments generated: 1
Review effort level: Lite

`test-results/.last-run.json` went in with the previous commit. Mine, and avoidable twice over: I ran `npx playwright test test/e2e/docker.spec.ts` without `--config test/playwright.config.ts`, which is what CI passes, so Playwright used its default output directory instead of the build/ path the config sets - and then `git add -A` swept the result in. Removed, with the two default paths ignored so a run that forgets the config flag cannot commit its state either. Verified by running it that way again: the directory comes back, git does not see it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Copilot

🔵 Needs a closer look

Registry credential helper lookup likely fails when credentials are configured under a non-primary config key due to always using keys[0] as the helper/serveraddress, which can break private-registry pulls.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:121

resolveRegistryAuth() selects a per-registry helper by scanning multiple possible config keys, but always passes keys[0] as the serverUrl to the helper (and as serveraddress in the encoded auth). If the helper/credential is configured under a different key (e.g. https://<registry>/v1/), this can cause helper lookups (and/or the daemon’s auth matching) to fail even though a credential exists.
src/main/docker/docker-filter-proxy.ts:318
The doc comment above decide() says it returns null when the request may proceed, but decide() now returns an object containing a refusal (or null) plus an optional rewritten body. This mismatch can cause incorrect usage when the code is modified later.

Files reviewed: 25/27 changed files
Comments generated: 0 new
Review effort level: Lite

…arn its job Three things, all of them gaps rather than bugs. The docker policy's network path had no real-CLI coverage anywhere. That is the one path where fixture-driven tests have now been wrong twice: the allowlist was built from the spec instead of the wire, so a create body the unit tests accepted was refused on the socket, and again for the CLI keys the second time. The e2e now creates a network, inspects it by the name the job chose, joins a container to it and removes it, plus the two refusals - an undeclared name, and a name declared internal asked for routable. It runs on both legs, so it drives the production proxy inside a localmost job and this file's own proxy outside one. That needs the repository to declare the network, so .localmostrc gains `localmost-e2e-*`, internal. The localmost runner will treat this as policy drift until the new .localmostrc is approved in the app; the localmost-legged jobs fail until then, by design. docker.yaml's check job was consumed by nothing: docker-linux and docker-localmost both hardcode their runner, and neither declared `needs: check`. Rather than delete a job whose output is worth having, docker-linux now reads it and says out loud when there is no heartbeat - that a queued docker-localmost is waiting for a runner, not broken. It is the job that can say it, since it always runs and docker-localmost by design may not. Neither job's runner changes: docker-localmost stays self-hosted and still queues rather than skipping. Three check jobs appear in the PR checks list and two of them displayed identically as "check / check". They now carry distinct names - "ci runner", "docker runner", "inline runner". Job ids are untouched, so `needs: check` still resolves, and no ruleset pins a status check name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Copilot

🔵 Needs a closer look

There are at least two correctness issues in the new/changed code (a Jest test file that may not have jest in scope, and registry-helper auth lookup using a potentially wrong server URL key) that should be fixed before merging.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:106

Registry helper lookup can select a helper configured under a non-primary key (e.g. https://<registry>), but still calls the helper with serveraddress = keys[0]. That makes the helper query the wrong server URL and can silently drop auth for registries that Docker stores under the alternate keys.
src/main/ipc-handlers/policy.test.ts:1
This test uses jest.mock/jest.fn() but doesn’t import jest from @jest/globals, unlike other Jest tests in this repo that use module mocking. If jest isn’t provided globally in the test runtime, this will throw before the tests run.

Files reviewed: 29/31 changed files
Comments generated: 0 new
Review effort level: Lite

`localmost policy approve` writes the whole .localmostrc to the cache, docker section included, and the runner enforces every bit of it. But `localmost policy show` rendered network, filesystem and env only: its PrintablePolicy type had no docker field at all, so images, mounts, networks and privileged could never print. An operator approving from the CLI never saw the container grants. The diff path does not save it either. The runner caches a policy when it first reads it, so by the time anyone runs `approve` the cached config already matches the working tree and diffConfigs has nothing to show. That is how it went here: the new networks entry was cached, unapproved, the instant CI picked the job up. Same class as the two the consumer found - a rule that is enforced but invisible where it is meant to be reviewed. The app already had a complete describer, so this moves it to src/shared/docker-policy.ts as describeDockerGrants and points both at it: the CLI and the app now describe one policy the same way rather than keeping two renderers that can drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Copilot

🔵 Needs a closer look

evaluateNetworkCreate uses only the first matching run.networks declaration, which can incorrectly deny routable network creates when multiple matching declarations exist with different internal values.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/docker/docker-evaluator.ts:658

Network declarations can be additive (mergeNetworks dedupes by name+internal), so it’s possible for multiple run.networks entries to match the same requested network name with different internal values. Using declared.find(...) means only the first match is considered; this can incorrectly deny a routable create even if another matching declaration allows internal: false.

Consider matching against all declarations: deny only when no declarations match, and (for a routable request) deny only when all matching declarations are internal: true.

Files reviewed: 31/33 changed files
Comments generated: 0 new
Review effort level: Lite

Moving the docker describer to shared fixed one instance. The class was still there: three renderers enumerated the policy keys by hand and each dropped a different one. `docker:` never printed in the CLI. `env:` never printed in the app's approval summary, so environment grants were approved unseen. The app still described `sockets:`, which the grammar stopped accepting. And `secrets:` - which workflow scope does accept and which decides what a job can read - printed nowhere at all. src/shared/policy-describe.ts now enumerates the keys once and returns structured grants. Presentation stays with the caller: `group` and `marker` for the CLI's grouped listing, `summary` for the app's flat one, so both show the same policy without a second enumeration to drift. PolicyApprovals renders the IPC grants, so the UI follows. The other half is validation. validatePolicy ignored any key it did not recognise, so `dokcer:` parsed clean, granted nothing, and appeared in no approval diff because no parser produced it - the exact shape of the `build:` defect a consumer reported. Unknown keys are now refused, naming the accepted ones, from the same list the describer walks. Scoped: a workflow may declare `secrets`, the shared section may not. That list is what stops this recurring. The guard test asserts every key in it produces a grant, so a key cannot be added to the grammar, be enforced, and stay invisible where it is meant to be read. It earned that immediately - `secrets` was missing from my first list, and the test found it rather than a person doing so later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Copilot

🔵 Needs a closer look

It makes wide, security-sensitive changes across the Docker filter/proxy/evaluator surface and needs final human review despite strong test additions.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:106

resolveRegistryAuth always passes keys[0] as serveraddress to the credential helper. If credHelpers is configured under a different key variant (e.g. https://<registry>/v1/), the helper lookup will succeed but the helper will be queried with a different serverUrl than the key it was configured for, which can miss the stored credential.

This issue also appears on line 108 of the same file.

src/main/docker/registry-auth.ts:112

When falling back to an inline auths entry, the encoded AuthConfig uses serveraddress = keys[0] even if the matching entry was found under a different key variant. This can produce an X-Registry-Auth payload whose serveraddress doesn’t match the config entry that provided the credential.

  for (const key of keys) {
    const entry = config.auths?.[key];
    if (!entry) continue;
    if (entry.identitytoken) return encode({ identitytoken: entry.identitytoken, serveraddress });
    if (!entry.auth) continue;

Files reviewed: 34/36 changed files
Comments generated: 0 new
Review effort level: Lite

docker-linux was given `needs: check` so it could say when there is no localmost heartbeat. That was make-work to justify a job whose output docker.yaml does not otherwise consume, and it cost what coupling costs: when the check job was cancelled, docker-linux was cancelled with it, though it runs GitHub-hosted whatever the heartbeat says. The fact belongs to the job that determines it. check.yaml already has a single point where it gives up on a local runner, so the warning goes there, and no caller needs a dependency to repeat it. Every caller gets it, which is right: a run silently falling back to a GitHub-hosted runner is worth seeing on a machine bought to run them locally. Not solved with `timeout-minutes` on docker-localmost, the other obvious candidate: that bounds execution, not time spent queued for a self-hosted runner, so a runner that never comes online still hangs until GitHub's own 24h limit. docker-linux is independent again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

Copilot

🔵 Needs a closer look

Registry auth resolution currently always uses keys[0] as serveraddress, which can mis-handle credentials stored under other common config keys (e.g. https://<registry>), potentially breaking authenticated pulls.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:99

serveraddress is always set to keys[0] (e.g. quay.io), but the helper can be configured under a different key (e.g. https://quay.io in credHelpers). In that case helper is found, but runHelper(helper, serveraddress) queries the wrong address and will return no credential even though one exists.

This issue also appears on line 108 of the same file.

src/main/docker/registry-auth.ts:112

When falling back to inline auths, the code always encodes serveraddress as keys[0], even if the matching config entry was found under a different key (e.g. https://quay.io or quay.io/v1/). This can produce an X-Registry-Auth payload whose serveraddress doesn’t correspond to the credential entry that was actually found.

  for (const key of keys) {
    const entry = config.auths?.[key];
    if (!entry) continue;
    if (entry.identitytoken) return encode({ identitytoken: entry.identitytoken, serveraddress });
    if (!entry.auth) continue;

Files reviewed: 35/37 changed files
Comments generated: 0 new
Review effort level: Lite

bfulton and others added 19 commits September 6, 2026 00:42

Copilot AI lite review requested due to automatic review settings September 6, 2026 20:03

Copilot started reviewing on behalf of bfulton September 6, 2026 20:03 View session